-
Notifications
You must be signed in to change notification settings - Fork 369
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: GitHub integration tagging issues #4586
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Skipped Deployments
|
Docker builds report
|
Uffizzi Ephemeral Environment
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4586 +/- ##
=======================================
Coverage 97.21% 97.21%
=======================================
Files 1171 1172 +1
Lines 40651 40662 +11
=======================================
+ Hits 39517 39528 +11
Misses 1134 1134 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to be testing this on staging post merge.
api/integrations/github/github.py
Outdated
@@ -100,8 +116,10 @@ def handle_github_webhook_event(event_type: str, payload: dict[str, Any]) -> Non | |||
handle_installation_deleted(payload) | |||
elif event_type in tag_by_event_type: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is pointless, if event_type
isn't in tag_by_event_type
then we'll get a KeyError
here. I assume based on that, there is no way for event_type
to not exist in tag_by_event_type
but we should check on this.
If that is the case, I think this can just be changed to (and we can remove one layer of nesting):
elif action in tag_by_event_type[event_type]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked and it was already filtered in the view code, so the check here is unnecessary. I've updated the relevant code.
Thanks for submitting a PR! Please check the boxes below:
docs/
if required so people know about the feature!Changes
Solve #4585 and #4584
Unit tests updated